Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

[WIP] Fix RedHat System.Globalization failures #24338

Closed
wants to merge 1 commit into from

Conversation

krwq
Copy link
Member

@krwq krwq commented Sep 29, 2017

This is WIP - I do not have RedHat to test, so currently using CentOS
https://github.com/dotnet/corefx/issues/24304

Original fix made by @tarekgh will require changes to CoreCLR (add ICU version detection to native code) which is likely not appropriate as servicing change which does not fix any product bugs.

Currently I do not understand why exactly RedHat 6.9 uses code path which is conditioned for Win10/OSX15+/Ubuntu14.04+/Fedora/Debian>8 and will investigate here - current state is sanity check to ensure straight forward change will pass on rhel and it is not i.e. Xunit swapping Expected/Actual labels.

@krwq krwq self-assigned this Sep 29, 2017
@krwq
Copy link
Member Author

krwq commented Sep 29, 2017

@dotnet/eng-first are there currently any known issues with the CI on 2.0 branch? It doesn't seem to have started.

@krwq krwq requested review from CIPop and removed request for CIPop September 29, 2017 17:34
@mmitche
Copy link
Member

mmitche commented Sep 29, 2017

@krwq The dev/release/2.0.0 branch isn't a tracked branch. Is this where you meant to make your PR?

@tarekgh
Copy link
Member

tarekgh commented Sep 29, 2017

Currently I do not understand why exactly RedHat 6.9 uses code path which is conditioned for Win10/OSX15+/Ubuntu14.04+/Fedora/Debian>8

on Linux, it depends on what version of ICU is used on such distro with a specific version. On Windows, Windows is changing/adding globalization data almost every release.
So Looks Red Hat 6.9 is using ICU version which is used in Win10/OSX15+/Ubuntu14.04+/Fedora/Debian>8

@tarekgh tarekgh self-requested a review September 29, 2017 17:38
@tarekgh
Copy link
Member

tarekgh commented Sep 29, 2017

👍

@krwq
Copy link
Member Author

krwq commented Sep 29, 2017

@tarekgh seems like I have swapped Expected value with Actual when reading and reversed condition (should be || Rhel6.9)

@tarekgh
Copy link
Member

tarekgh commented Sep 29, 2017

seems like I have swapped Expected value with Actual when reading and reversed condition (should be || Rhel6.9)

The expected value for the test case is [3, 2]. so I think using !redhat6.9 is correct.

@krwq
Copy link
Member Author

krwq commented Sep 29, 2017

@tarekgh I believe [3] is correct expected value - that is what Win10/ICU 55+ is using (also what you wrote above) - !RedHat6.9 wouldn't change test result as we currently already expect [3,2] and seems like RHEL6.9 used by CI have ICU 55+. (note that UrINNumberGroupSizes returns expected value not actual and there is no reason any of the conditions in the if statement would be met)
https://mc.dot.net/#/product/netcore/200/source/official~2Fcorefx~2Frelease~2F2.0.0~2F/type/test~2Ffunctional~2Fcli~2F/build/20170927.01

@krwq
Copy link
Member Author

krwq commented Sep 29, 2017

Closing this in favor of #24340 - this PR is improperly targeting dev/release/2.0.0

@krwq krwq closed this Sep 29, 2017
@tarekgh
Copy link
Member

tarekgh commented Sep 29, 2017

@krwq
the expected value is Expected: Int32[] [3, 2]

2017-09-27 13:43:11,451: INFO: proc(54): run_and_log_output: Output:    System.Globalization.Tests.NumberFormatInfoNumberGroupSizes.NumberGroupSizes_Get(format: NumberFormatInfo { CurrencyDecimalDigits = 2, CurrencyDecimalSeparator = \".\", CurrencyGroupSeparator = \",\", CurrencyGroupSizes = [3, 2], CurrencyNegativePattern = 9, ... }, expected: [3, 2]) [FAIL]
2017-09-27 13:43:11,453: INFO: proc(54): run_and_log_output: Output:       Assert.Equal() Failure
2017-09-27 13:43:11,453: INFO: proc(54): run_and_log_output: Output:       Expected: Int32[] [3, 2]
2017-09-27 13:43:11,453: INFO: proc(54): run_and_log_output: Output:       Actual:   Int32[] [3]
2017-09-27 13:43:12,240: INFO: proc(54): run_and_log_output: Output: Finished:    System.Globalization.Tests

@krwq
Copy link
Member Author

krwq commented Sep 29, 2017

@tarekgh true, I think we got both confused by this condition:
UrINNumberGroupSizes - returns expected value [3] when Win10 or ICU 55+ otherwise [3,2]
RHEL 6.9 - CI uses ICU 55+ and the actual value in the failing test is [3] as it should be
current version of the code running on RHEL 6.9 is expecting [3,2] because none of the conditions is satisfied for RHEL and we get into the else part ([3,2]).

@joperezr joperezr mentioned this pull request Oct 5, 2017
@karelz karelz added this to the 2.0.x milestone Feb 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants